fix(agent): fix potential memleak when calling newrelic_end_transaction(true) #1072
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1072 +/- ##
=======================================
Coverage 78.11% 78.12%
=======================================
Files 193 193
Lines 27991 27999 +8
=======================================
+ Hits 21866 21875 +9
+ Misses 6125 6124 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ZNeumann
force-pushed
the
zjn/valgrind
branch
2 times, most recently
from
5deff4d to
e953a97
Compare
agent/php_mysqli.h
Outdated
| * Purpose : Frees reference incremented, transaction global zvals | ||
| * that must be cleaned up prior to postdeactivate | ||
| */ | ||
| extern void nr_mysqli_rshutdown(); |
There was a problem hiding this comment.
There's a naming convention that symbols that are PHP runtime aware and are defined in the agent have the prefix of nr_php_ and the functions that are agnostic of PHP runtime and are defined in axiom have the prefix of nr_. This convention helps in reasoning about the code when reading it. Please update the name of this function to nr_php_mysqli_rshutdown.
There was a problem hiding this comment.
changed in 5b8dd71
agent/php_pdo.h
Outdated
| * Purpose : Frees reference incremented, transaction global zvals | ||
| * that must be cleaned up prior to postdeactivate | ||
| */ | ||
| extern void nr_pdo_rshutdown(); |
There was a problem hiding this comment.
There's a naming convention that symbols that are PHP runtime aware and are defined in the agent have the prefix of nr_php_ and the functions that are agnostic of PHP runtime and are defined in axiom have the prefix of nr_. This convention helps in reasoning about the code when reading it. Please update the name of this function to nr_php_pdo_rshutdown.
There was a problem hiding this comment.
changed in 5b8dd71
Some transaction globals were leaked when calling
newrelic_end_transaction(true). The freeing of these transaction globals has been moved to join all the other transaction globals to alleviate this.In so doing, additional rshutdown methods are needed. This is because the transaction globals contain reference incremented zvals. If we wait until postdeactivate, those zvals will be in an undefined state and the data structures containing those references cannot be properly freed. In line with other transaction globals, we trigger a shutdown method during rshutdown to free these globals while the zvals are still valid.